LUN-400: Image for own vocabulary #377

ztefanie · 2022-10-04T12:33:36Z

This pull request belongs to an issue on our bugtracker.
You can find it there by looking for an issue with the key which is mentioned in the title of this pull request.
It starts with the keyword LUN.

Should work for Android and iOS. Saves images in app specific storage for both.

android/app/src/main/java/app/lunes/MainApplication.java

src/components/CameraOverlay.tsx

src/components/__tests__/CameraOverlay.spec.tsx

charludo

Looks like the failing tests are due to the same error we've talked about in Lun-407 recently.

Adding jest.mock('@react-navigation/native') here should fix it.

# Conflicts: # src/routes/user-vocabulary-list/UserVocabularyListScreen.tsx # src/services/AsyncStorage.ts

# Conflicts: # src/components/NotAuthorisedView.tsx # src/routes/add-custom-discipline/components/QRCodeReaderOverlay.tsx

steffenkleinle

Looks really good already, well done! Tested on android.

Things I noticed:

The error messages of the image overlay are not adjusted, e.g. it says Kamerazugriff zum Scannen von QR-Codes erforderlich even though I am trying to add images for vocabulary.
There is no kind of feedback that I already took an image. I think it would be better if the camera overlay would close after taking one image and the user has to klick on Bild hochladen again if more than one images are wanted (I think it will not be that common)
Bild hochladen sounds like it is uploaded to a server or something which I personally as a privacy oriented user would be very hesitant to do. Instead, the naming Bild hinzufügen would be better imo. Not sure if we should use plural.
Is it planned to add an option to add images from the gallery instead of having to take new pictures?
What is the small image button in the bottom left of the camera view doing? Nothing happens for me and I got a unhandled promise rejection once upon clicking it.
The view for user vocabulary is broken, see screenshot (probably not done in your PR though).

src/services/helpers.ts

src/routes/process-user-vocabulary/components/ImageSelectionOverlay.tsx

src/components/CameraOverlay.tsx

src/routes/process-user-vocabulary/components/Thumbnail.tsx

src/routes/process-user-vocabulary/UserVocabularyProcessScreen.tsx

src/routes/process-user-vocabulary/components/ImageSelectionOverlay.tsx

src/routes/process-user-vocabulary/UserVocabularyProcessScreen.tsx

ztefanie · 2022-10-06T06:48:02Z

The error messages of the image overlay are not adjusted, e.g. it says Kamerazugriff zum Scannen von QR-Codes erforderlich even though I am trying to add images for vocabulary.

Good catch. Fixed.

There is no kind of feedback that I already took an image. I think it would be better if the camera overlay would close after taking one image and the user has to klick on Bild hochladen again if more than one images are wanted (I think it will not be that common)

Good point. There is feedback (audio shutter sound), but closing is completely fine too. Changed it.

Bild hochladen sounds like it is uploaded to a server or something which I personally as a privacy oriented user would be very hesitant to do. Instead, the naming Bild hinzufügen would be better imo. Not sure if we should use plural.

Naming is a little difficult always, as we try only to use A2 or at least really simple german. I changed to "hinzufügen" anyways.

Is it planned to add an option to add images from the gallery instead of having to take new pictures?

Yes will be done in separate ticket (as mentioned as a todo in the comment).

What is the small image button in the bottom left of the camera view doing? Nothing happens for me and I got a unhandled promise rejection once upon clicking it.

Used for selection from gallery, will be done in separate ticket.

The view for user vocabulary is broken, see screenshot (probably not done in your PR though).

Not happend in my PR, already fixed this in LUN-420.

sarahsporck

Tested on Android.

assets/images/index.ts

src/routes/process-user-vocabulary/components/ImageSelectionOverlay.tsx

src/routes/process-user-vocabulary/components/Thumbnail.tsx

sarahsporck · 2022-10-06T07:54:25Z

Not happend in my PR, already fixed this in LUN-420.

Created and already rejected an issue for this (LUN-444). I thought this happened because of my phone resolution ^^. Also it seems, I had a really bad timing when reviewing this PR.

f1sh1918

Tested on android and ios
ListItem.tsx line 23, pls add flex: 1; to fix listItem fullwidth issue
The rest steffen mentioned is fine for me since select image from gallery is a separate task. Maybe you could just comment out the gallery icon for now to not confuse testers

f1sh1918

Tested on android (real device) and ios (real device).
Android works fine.
On iOS the images were not shown taken by camera. (real device & emulator)

Additionally there is an initCap for the Word missing (bar)

Looks like document-image couldn't be saved correctly (empty array)
ListItem should be fixed as steffen mentioned (ListItem line 23) width: 100%;
(added it for my screenshot to show missing thumbnail)

steffenkleinle

Tested on android, great job 🖼️

src/routes/process-user-vocabulary/UserVocabularyProcessScreen.tsx

src/routes/process-user-vocabulary/components/ImageSelectionOverlay.tsx

sarahsporck · 2022-10-10T06:48:36Z

When using the article Dropdown I get an error saying Closed Virtualizedlist should never be nested inside scrollview. This should be fixable like this: hossein-zare/react-native-dropdown-picker#56 (comment).
Would you mind if you'd add this line in this PR? :)

ztefanie · 2022-10-11T09:15:37Z

Tested on android (real device) and ios (real device). Android works fine. On iOS the images were not shown taken by camera. (real device & emulator)

Will have a look at this.

Additionally there is an initCap for the Word missing (bar)

Not part of this ticket. Add to LUN-401 if you think this should be done.

ztefanie · 2022-10-11T09:18:46Z

Tested on android and ios ListItem.tsx line 23, pls add flex: 1; to fix listItem fullwidth issue

Added with 100%, as you suggested in the other comment

The rest steffen mentioned is fine for me since select image from gallery is a separate task. Maybe you could just comment out the gallery icon for now to not confuse testers

Won't do, as this whole feature is wip. We also didn't do this for other icons, which do nothing yet.

ztefanie · 2022-10-11T09:41:09Z

When using the article Dropdown I get an error saying Closed Virtualizedlist should never be nested inside scrollview. This should be fixable like this: hossein-zare/react-native-dropdown-picker#56 (comment). Would you mind if you'd add this line in this PR? :)

Done and close the ticket @f1sh1918 opened for it.

f1sh1918

Could only test on ios simulator and that worked now. Can not build on real device atm

ztefanie · 2022-10-11T13:27:03Z

Tested on ios emulator and real devices again, works just fine.

ztefanie and others added 2 commits October 4, 2022 10:26

LUN-400: Image for own vocabulary

6d508c9

LUN-400: Image for own vocabulary ios

79183da

ztefanie requested review from steffenkleinle, sarahsporck and f1sh1918 as code owners October 4, 2022 12:33

ztefanie commented Oct 4, 2022

View reviewed changes

android/app/src/main/java/app/lunes/MainApplication.java Show resolved Hide resolved

ztefanie commented Oct 4, 2022

View reviewed changes

src/components/CameraOverlay.tsx Show resolved Hide resolved

ztefanie commented Oct 4, 2022

View reviewed changes

src/components/__tests__/CameraOverlay.spec.tsx Show resolved Hide resolved

LUN-400: Fix import order

c6024e9

charludo reviewed Oct 5, 2022

View reviewed changes

ztefanie added 5 commits October 5, 2022 08:45

LUN-400: Fixed failing test

e60ca4f

Merge branch 'main' into LUN-400-image-for-own-vocabulary

ea80021

# Conflicts: # src/routes/user-vocabulary-list/UserVocabularyListScreen.tsx # src/services/AsyncStorage.ts

LUN-400: Resolve merge conflicts

807f8ba

Merge branch 'main' into LUN-400-image-for-own-vocabulary

d41db46

# Conflicts: # src/components/NotAuthorisedView.tsx # src/routes/add-custom-discipline/components/QRCodeReaderOverlay.tsx

LUN-400: Resolve merge conflict

6141ea4

ztefanie force-pushed the LUN-400-image-for-own-vocabulary branch from 96eefe4 to 6141ea4 Compare October 5, 2022 07:28

steffenkleinle requested changes Oct 5, 2022

View reviewed changes

LUN-400: Image for own vocabulary - improvements

a10d7b6

ztefanie requested review from steffenkleinle and charludo October 6, 2022 06:58

Merge branch 'main' into LUN-400-image-for-own-vocabulary

52893d7

sarahsporck approved these changes Oct 6, 2022

View reviewed changes

assets/images/index.ts Show resolved Hide resolved

src/routes/process-user-vocabulary/components/ImageSelectionOverlay.tsx Show resolved Hide resolved

src/routes/process-user-vocabulary/components/Thumbnail.tsx Show resolved Hide resolved

f1sh1918 reviewed Oct 6, 2022

View reviewed changes

f1sh1918 requested changes Oct 6, 2022

View reviewed changes

steffenkleinle approved these changes Oct 6, 2022

View reviewed changes

LUN-400: Adjustments for add image

dafd644

steffenkleinle approved these changes Oct 11, 2022

View reviewed changes

Merge branch 'main' into LUN-400-image-for-own-vocabulary

d2ef173

steffenkleinle requested a review from f1sh1918 October 11, 2022 11:37

f1sh1918 approved these changes Oct 11, 2022

View reviewed changes

ztefanie merged commit 2cb605c into main Oct 11, 2022

ztefanie deleted the LUN-400-image-for-own-vocabulary branch October 11, 2022 13:27

jira-to-github-migrator bot mentioned this pull request Aug 13, 2023

LUN-454: User Vocabulary List Item does not show thumbnail #887

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LUN-400: Image for own vocabulary #377

LUN-400: Image for own vocabulary #377

ztefanie commented Oct 4, 2022

charludo left a comment

steffenkleinle left a comment •

edited

Loading

ztefanie commented Oct 6, 2022

sarahsporck left a comment

sarahsporck commented Oct 6, 2022 •

edited

Loading

f1sh1918 left a comment •

edited

Loading

f1sh1918 left a comment •

edited

Loading

steffenkleinle left a comment

sarahsporck commented Oct 10, 2022

ztefanie commented Oct 11, 2022 •

edited

Loading

ztefanie commented Oct 11, 2022

ztefanie commented Oct 11, 2022

f1sh1918 left a comment

ztefanie commented Oct 11, 2022

LUN-400: Image for own vocabulary #377

LUN-400: Image for own vocabulary #377

Conversation

ztefanie commented Oct 4, 2022

charludo left a comment

Choose a reason for hiding this comment

steffenkleinle left a comment • edited Loading

Choose a reason for hiding this comment

ztefanie commented Oct 6, 2022

sarahsporck left a comment

Choose a reason for hiding this comment

sarahsporck commented Oct 6, 2022 • edited Loading

f1sh1918 left a comment • edited Loading

Choose a reason for hiding this comment

f1sh1918 left a comment • edited Loading

Choose a reason for hiding this comment

steffenkleinle left a comment

Choose a reason for hiding this comment

sarahsporck commented Oct 10, 2022

ztefanie commented Oct 11, 2022 • edited Loading

ztefanie commented Oct 11, 2022

ztefanie commented Oct 11, 2022

f1sh1918 left a comment

Choose a reason for hiding this comment

ztefanie commented Oct 11, 2022

steffenkleinle left a comment •

edited

Loading

sarahsporck commented Oct 6, 2022 •

edited

Loading

f1sh1918 left a comment •

edited

Loading

f1sh1918 left a comment •

edited

Loading

ztefanie commented Oct 11, 2022 •

edited

Loading